Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(controller): configurable terminationGracePeriodSeconds #4940

Merged
merged 4 commits into from
Feb 8, 2021
Merged

feat(controller): configurable terminationGracePeriodSeconds #4940

merged 4 commits into from
Feb 8, 2021

Conversation

tczhao
Copy link
Member

@tczhao tczhao commented Jan 25, 2021

allow configure terminationGracePeriodSeconds from template

It sets the max wait time between SIGTERM and SIGKILL for main and sidecars

Resolves: #1012

Checklist:

simster7
simster7 reviewed Jan 25, 2021
View reviewed changes
Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, but before merging I'll bring this up with the team to make sure we still want this spec. It has been a while since that issue was opened

@simster7 simster7 self-assigned this Jan 25, 2021
@tczhao tczhao force-pushed the feature/template-terminateGracePeriod branch from e733603 to 6dd5e17 Compare January 25, 2021 06:35
@alexec
Copy link
Contributor

alexec commented Jan 25, 2021

What’s the use case for this? You’ve sidecars that take >30s to terminate?

@tczhao
Copy link
Member Author

tczhao commented Jan 25, 2021

I personally don't have any use case for this,
but it seems there's a demand according to thread #1012 and a PR is asked

@tczhao
Copy link
Member Author

tczhao commented Jan 25, 2021

Hi @ryan4yin @fabio-rigato @kerneltime @matveybrant,

Could you give us some use cases on when a configurable terminationGracePeriod would be needed?

@simster7
Copy link
Member

simster7 commented Jan 27, 2021 •
edited
Loading

Hi @tczhao. We currently won't want to support this as its own field in our spec (i.e. as Workflow.spec.tempalte.terminationGracePeriodSeconds). Our current idea is to use the Pod.spec.terminationGracePeriodSeconds field for this. The user can set that field using podSpectPatch and the executor can read from that field instead to apply this logic. We decided on this because we don't want to commit our own types to this yet. Let us know if you have comments on this, otherwise please refactor the PR to this new spec

@tczhao
Copy link
Member Author

tczhao commented Jan 27, 2021

That make sense, will do the change

@tczhao tczhao force-pushed the feature/template-terminateGracePeriod branch from 6dd5e17 to 9890f1b Compare January 28, 2021 10:09
@simster7
Copy link
Member

simster7 commented Feb 2, 2021

@tczhao Bump! Still working on this?

@tczhao tczhao force-pushed the feature/template-terminateGracePeriod branch 3 times, most recently from 3541ec8 to 4b9b06f Compare February 3, 2021 00:02
@tczhao
Copy link
Member Author

tczhao commented Feb 3, 2021

@simster7 updated, please have a look

simster7
simster7 reviewed Feb 3, 2021
View reviewed changes
Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tczhao, this looks great thanks. Could you please include an E2E test that makes use of the podSpecPatch field? (example: https://github.com/argoproj/argo/blob/master/examples/pod-spec-patch.yaml).

This is crucial to ensure that a user can set this field right from their workflow

@tczhao
feat(controller): use terminationGracePeriodSeconds from podSpec
41af29d 
Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
@tczhao tczhao force-pushed the feature/template-terminateGracePeriod branch 3 times, most recently from b011a74 to 0ae3ffb Compare February 4, 2021 06:33
@tczhao
test(e2e): add podSpecPatch test
96c8279 
Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>
@tczhao tczhao force-pushed the feature/template-terminateGracePeriod branch from 0ae3ffb to 96c8279 Compare February 4, 2021 07:10
@tczhao
Copy link
Member Author

tczhao commented Feb 4, 2021

@simster7 e2e test added

@simster7 simster7 merged commit 5915a21 into argoproj:master Feb 8, 2021
@simster7 simster7 mentioned this pull request Feb 8, 2021
Closed
38 tasks
@simster7 simster7 mentioned this pull request Feb 16, 2021
Closed
33 tasks
@alexec alexec mentioned this pull request Mar 11, 2022
Merged
@tooptoop4 tooptoop4 mentioned this pull request Oct 3, 2024
Open
4 tasks
@agilgur5 agilgur5 mentioned this pull request Oct 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simster7 simster7 simster7 approved these changes

@jessesuen jessesuen Awaiting requested review from jessesuen

Assignees

@simster7 simster7

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configurable graceful shutdown period for container and sidecar
3 participants
@tczhao @alexec @simster7